Skip to content

Add test for "Regression in v3.8.x PopulateFeedBacklog depends on un-started InboxReader" #3847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 16, 2025
Merged

Add test for "Regression in v3.8.x PopulateFeedBacklog depends on un-started InboxReader" #3847

merged 4 commits into from
Oct 16, 2025

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Oct 15, 2025 •
edited
Loading

Fixes NIT-4014

Problem

We fixed NIT-3997 "Regression in v3.8.x PopulateFeedBacklog depends on un-started InboxReader", but want to add a regression test that the fix did work, and a similar bug doesn't happen again

The fix was done in this PR

Solution

I added a regression test it passes on master (i.e. this branch), but fails before the original fix was merged.

To replicate this test failing more the fix was in place you can checkout this branch https://github.com/KolbyML/nitro/tree/old-add-regresion-test

Then run make go-test, cancel it after the test setup completes aka tests start running, then run go test ./system_tests -run TestRegressionInPopulateFeedBacklog -v -count=1 -timeout=10m and it will fail with error message

    common_test.go:687:  [] error populating feed backlog on startup: error getting message 1: failed to fetch batch mentioned by batch posting report: not started

@KolbyML KolbyML force-pushed the new-add-regression-test branch from 968ebdc to 4f688d7 Compare October 15, 2025 17:33
pmikolajczyk41
pmikolajczyk41 reviewed Oct 15, 2025
View reviewed changes
system_tests/seqfeed_test.go
builder.L2Info.GenerateAccount(userAccount)

// Guarantees that nodes will rely only on the feed to receive messages
builder.nodeConfig.BatchPoster.Enable = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we recently discovered, this will turn batch poster everywhere and should be adapted to #3829; bringing @ganeshvanahalli attention in case this PR will get merged first

@pmikolajczyk41
Copy link
Member

@KolbyML I'm afraid that the Aggregate test results fails because your fork doesn't have the codecov token secret. Probably you just have to use the original repo for PRs (though it's not a required check)

@KolbyML
Copy link
Member Author

KolbyML commented Oct 15, 2025

@pmikolajczyk41 ready for another look 🫡

pmikolajczyk41
pmikolajczyk41 previously approved these changes Oct 15, 2025
View reviewed changes
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's really great to have steps for reproduction, but (apologies for nitpicking), it'd be more straightforward to make old-add-regression-test branch differ from the actual branch by the fix we are testing (i.e., #3812); currently these branches have much more differences (KolbyML/nitro@new-add-regression-test...old-add-regresion-test and KolbyML/nitro@old-add-regresion-test...new-add-regression-test)

I run the test with and without fix and I confirm that the test does its job :D

@KolbyML
Copy link
Member Author

KolbyML commented Oct 15, 2025

it's really great to have steps for reproduction, but (apologies for nitpicking), it'd be more straightforward to make old-add-regression-test branch differ from the actual branch by the fix we are testing (i.e., #3812); currently these branches have much more differences (KolbyML/[email protected] and KolbyML/[email protected])

good to know I will keep that in mind for future PR's 🫡

I run the test with and without fix and I confirm that the test does its job :D

🥳

@KolbyML KolbyML assigned eljobe and unassigned amsanghi Oct 15, 2025
@pmikolajczyk41 pmikolajczyk41 mentioned this pull request Oct 16, 2025
Merged
@eljobe eljobe enabled auto-merge October 16, 2025 14:25
@eljobe eljobe added this pull request to the merge queue Oct 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2025
@pmikolajczyk41 pmikolajczyk41 added this pull request to the merge queue Oct 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 16, 2025
@KolbyML KolbyML added this pull request to the merge queue Oct 16, 2025
Merged via the queue into OffchainLabs:master with commit 1026501 Oct 16, 2025
25 checks passed
@KolbyML KolbyML deleted the new-add-regression-test branch October 16, 2025 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pmikolajczyk41 pmikolajczyk41 pmikolajczyk41 approved these changes

@diegoximenes diegoximenes Awaiting requested review from diegoximenes

@Tristan-Wilson Tristan-Wilson Awaiting requested review from Tristan-Wilson

@alexandrosfilios alexandrosfilios Awaiting requested review from alexandrosfilios

@amsanghi amsanghi Awaiting requested review from amsanghi

@eljobe eljobe Awaiting requested review from eljobe

Assignees

@eljobe eljobe

Labels

design-approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KolbyML @pmikolajczyk41 @eljobe @amsanghi